Skip to content

optimize(api): add warning logs for load-based request rejection#2972

Open
contrueCT wants to merge 5 commits intoapache:masterfrom
contrueCT:fix/issue-2747-load-detect-logs
Open

optimize(api): add warning logs for load-based request rejection#2972
contrueCT wants to merge 5 commits intoapache:masterfrom
contrueCT:fix/issue-2747-load-detect-logs

Conversation

@contrueCT
Copy link
Copy Markdown
Contributor

@contrueCT contrueCT commented Mar 19, 2026

Purpose of the PR

Main Changes

Log busy-worker and low-memory rejections in LoadDetectFilter before returning ServiceUnavailableException so operators can diagnose 503 responses from server-side logs.

Add unit coverage for whitelist bypass, busy rejection,
low-memory rejection, and healthy request pass-through, and register the new test in UnitTestSuite.

Verifying these changes

  • Trivial rework / code cleanup without any test coverage. (No Need)
  • Already covered by existing tests, such as (please modify tests here).
  • Need tests and can be verified as follows:
    • xxx

Does this PR potentially affect the following parts?

Documentation Status

  • Doc - TODO
  • Doc - Done
  • Doc - No Need

@imbajin imbajin requested a review from Copilot March 20, 2026 09:03
Copilot AI reviewed Mar 20, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds operator-visible warning logs when LoadDetectFilter rejects requests due to high worker load or low free memory (503s), and introduces unit tests to cover the new rejection behavior plus whitelist/healthy pass-through.

Changes:

  • Add WARN logging for worker-load and low-memory request rejections in LoadDetectFilter.
  • Add LoadDetectFilterTest unit coverage for whitelist bypass, busy rejection, low-memory rejection, and healthy requests.
  • Register the new unit test in UnitTestSuite.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/LoadDetectFilter.java Adds rejection WARN logs and minor refactor (captures current load).
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/api/filter/LoadDetectFilterTest.java New unit tests for whitelist/busy/low-memory/healthy scenarios.
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/UnitTestSuite.java Adds LoadDetectFilterTest to the unit suite.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@imbajin imbajin requested a review from Copilot March 23, 2026 05:57
Copilot AI reviewed Mar 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

VGalaxies
VGalaxies reviewed Mar 23, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@VGalaxies VGalaxies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I left two small review comments; CI looks green on my side, but I think these are still worth addressing.

@contrueCT
Copy link
Copy Markdown
Contributor Author

contrueCT commented Mar 23, 2026

Thanks for the update. I left two small review comments; CI looks green on my side, but I think these are still worth addressing.

⚠️ The busy-load branch is rate-limited, but this low-memory WARN will still fire on every rejected request. Under sustained memory pressure that can flood the logs and add more I/O pressure to an already stressed server. Please apply the same REJECT_LOG_RATE_LIMITER here, or route both rejection logs through a shared sampling helper.
⚠️ This test drives the real System.gc() path by forcing the low-memory branch. That makes the suite slower and can make it behave differently on constrained CI nodes or JVMs with different GC ergonomics. Consider adding a test seam for the GC step so the test can stub it and only verify the rejection behavior.

Changes made:

  • Applied the same reject-log rate limiting to the low-memory branch, and routed both rejection paths through a shared warning helper so the sampling behavior is consistent.
  • Added a small test seam for gcIfNeeded() / reject-log sampling so the unit test no longer depends on a real System.gc() call.
  • Expanded LoadDetectFilterTest to verify:
    • busy rejection emits a warning log
    • low-memory rejection emits a warning log
    • whitelist / healthy paths do not emit rejection logs
    • reject-log sampling suppresses repeated warning logs as expected

@imbajin imbajin requested a review from Copilot March 24, 2026 03:26
Copilot AI reviewed Mar 24, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 2, 2026
@contrueCT contrueCT requested a review from imbajin April 2, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@VGalaxies VGalaxies VGalaxies left review comments

Copilot code review Copilot Copilot left review comments

@imbajin imbajin Awaiting requested review from imbajin

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Improvement] Add critical log statements.

4 participants

@contrueCT @imbajin @VGalaxies